-
-
Notifications
You must be signed in to change notification settings - Fork 725
Improve the update CLI command and fix missing tsconfig.json error #1315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: fcf34c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Caution Review failedThe pull request is closed. WalkthroughThe changes introduced in this pull request focus on improving the development experience by enhancing error handling, modifying command behaviors, and adding new configuration options. Key updates include a mechanism to gracefully handle missing Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .changeset/clever-buses-watch.md (1 hunks)
- .changeset/soft-ladybugs-promise.md (1 hunks)
- packages/cli-v3/src/commands/dev.ts (1 hunks)
- packages/cli-v3/src/commands/login.ts (2 hunks)
- packages/cli-v3/src/commands/update.ts (5 hunks)
- packages/cli-v3/src/commands/whoami.ts (2 hunks)
- packages/cli-v3/src/config.ts (3 hunks)
- packages/cli-v3/src/utilities/sourceFiles.ts (1 hunks)
- packages/core/src/v3/build/resolvedConfig.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/cli-v3/src/utilities/sourceFiles.ts
Additional comments not posted (14)
.changeset/soft-ladybugs-promise.md (1)
1-5
: Changeset looks good!The changeset clearly describes the purpose and impact of the changes made to the
trigger.dev
CLI tool. The fixes for the update command and the modification to hide the "whoami" command output in dev mode seem to be focused on improving the user experience and streamlining the development workflow..changeset/clever-buses-watch.md (1)
1-6
: LGTM!The changeset file looks good. The metadata and description align with the PR objectives and the AI-generated summary.
packages/core/src/v3/build/resolvedConfig.ts (1)
27-27
: LGTM!The addition of the optional
tsconfigPath
property to theResolvedConfig
type is a valid enhancement. It allows specifying a path to a TypeScript configuration file, which can be useful for projects that utilize TypeScript in their build processes. The change expands the configurability of theResolvedConfig
type without breaking existing code.packages/cli-v3/src/commands/whoami.ts (1)
54-55
: LGTM!The addition of the
silent
parameter and the corresponding conditional checks for controlling the loading spinner messages are implemented correctly. This change enhances the flexibility of thewhoAmI
function by allowing it to operate in a silent mode when needed, without introducing any apparent issues.Also applies to: 63-65, 71-71, 74-79, 118-118
packages/cli-v3/src/commands/dev.ts (1)
54-54
: LGTM! Thesilent
parameter provides flexibility in controlling login output verbosity.The addition of the
silent: true
parameter to thelogin
function call allows for suppressing the output typically displayed during the login operation. This can be beneficial in scenarios where a quieter execution is desired, such as in automated scripts or background processes.However, please ensure that critical error messages or important login-related information is still communicated to the user when necessary, even in silent mode. This will help maintain a balance between verbosity control and providing essential feedback to the user.
packages/cli-v3/src/config.ts (2)
141-141
: LGTM!The change to use the new
safeResolveTsConfig
function instead ofresolveTSConfig
is a good improvement. It will handle potential errors more gracefully during the resolution of the TypeScript configuration file.
191-197
: Great addition of thesafeResolveTsConfig
function!The
safeResolveTsConfig
function is a nice addition to handle potential errors gracefully when resolving the TypeScript configuration file. Wrapping theresolveTSConfig
function in a try-catch block and returningundefined
if an error occurs is a good approach to improve the robustness of the configuration resolution process.This change aligns with the AI-generated summary and enhances the overall error handling in the codebase.
packages/cli-v3/src/commands/login.ts (3)
62-62
: LGTM!The new optional
silent
property is correctly added to theLoginOptions
type to control the verbosity of the login command. Making it optional ensures backward compatibility.
68-73
: Looks good!The
opts
object is correctly defined with default values, includingsilent
set tofalse
. Spreading theoptions
object allows the caller to override these defaults. This ensures that the login command remains verbose by default unless explicitly silenced.
120-121
: Verify thewhoAmI
function.Please ensure that:
- The
whoAmI
function signature is updated to accept the newsilent
argument.- The
whoAmI
function implementation handles thesilent
argument correctly to control its verbosity.Run the following script to verify the
whoAmI
function:packages/cli-v3/src/commands/update.ts (4)
130-130
: LGTM!The change from "Nothing to do" to "Nothing to update" improves clarity and aligns with the action being performed.
Line range hint
136-209
: LGTM!The updated error handling and user confirmation logic based on the
embedded
flag enhances the user experience by providing appropriate messages and actions in different scenarios.
46-46
: Verify the impact of passingfalse
for theembedded
flag.The change looks good, but please ensure that explicitly passing
false
as theembedded
flag toupdateTriggerPackages
aligns with the expected behavior and doesn't introduce any unintended side effects.Run the following script to analyze the usage of the
embedded
flag inupdateTriggerPackages
:Verification successful
To continue our verification process, let's search for the implementation of
updateTriggerPackages
and analyze how it uses theembedded
parameter.
To complete our verification, we need to examine how the
embedded
parameter is used within theupdateTriggerPackages
function. Let's search for its usage.
The change is safe and doesn't impact the behavior of
updateTriggerPackages
.After thorough investigation, we can confirm that explicitly passing
false
for theembedded
flag inupdateCommand
does not affect the execution ofupdateTriggerPackages
. Theembedded
parameter is not used within the function body, suggesting it might be a remnant from previous versions or reserved for future use.Consider removing the
embedded
parameter from theupdateTriggerPackages
function signature if it's not used elsewhere in the codebase, as this would improve code clarity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the usage of the `embedded` flag in `updateTriggerPackages`. # Test: Search for conditional logic based on the `embedded` flag. Expect: Relevant code snippets. rg --type typescript -A 5 $'if \(embedded\)' $(fd -e ts -E node_modules -E dist -E build -E coverage -E .git)Length of output: 42758
Script:
#!/bin/bash # Search for the implementation of updateTriggerPackages rg -A 10 -B 5 "function updateTriggerPackages" packages/cli-v3/src/Length of output: 1163
Script:
#!/bin/bash # Search for the usage of 'embedded' within updateTriggerPackages rg -A 10 -B 10 "function updateTriggerPackages" packages/cli-v3/src/commands/update.ts | rg "embedded"Length of output: 124
Line range hint
220-259
: Verify the impact of silent package installation.The changes to the spinner messages improve clarity and provide more context to the user. However, please ensure that running the package installation silently at line 254 doesn't hide any important information or errors that the user should be aware of.
Run the following script to analyze the impact of silent package installation:
Verification successful
Silent installation is handled with appropriate error messaging.
The
installDependencies
function is called withsilent: true
, but it's wrapped in a try-catch block. If an error occurs during installation, a failure message is displayed to the user. Additionally, there's anexitHandler
mechanism that seems to handle cleanup or warnings about manual rollbacks.While the silent installation might hide some information during the process, the error handling ensures that users are informed if something goes wrong. The implementation strikes a balance between a clean user experience and necessary error reporting.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the impact of silent package installation. # Test: Search for error handling logic related to package installation. Expect: Relevant code snippets. rg --type typescript -A 10 $'installDependencies\(' $(fd -e ts -E node_modules -E dist -E build -E coverage -E .git)Length of output: 42765
Script:
#!/bin/bash # Search for installDependencies function and its usage rg -i "function\s+installDependencies|installDependencies\s*\(" packages/cli-v3/src # Search for error handling related to package installation rg -i "catch.*install|error.*install" packages/cli-v3/srcLength of output: 506
Script:
#!/bin/bash # Check the implementation of installDependencies rg -A 10 "function\s+installDependencies" packages/cli-v3/src # Check usage of parseNpmInstallError rg "parseNpmInstallError" packages/cli-v3/src # Check error handling around installDependencies in update.ts rg -A 10 -B 5 "installDependencies" packages/cli-v3/src/commands/update.tsLength of output: 1588
@@ -179,7 +179,7 @@ async function resolveConfig( | |||
conditions: [], | |||
}, | |||
} | |||
); | |||
) as ResolvedConfig; // TODO: For some reason, without this, there is a weird type error complaining about tsconfigPath being string | nullish, which can't be assigned to string | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Investigate the root cause of the type error instead of using a type assertion.
The type assertion is used to suppress a type error related to the tsconfigPath
property. However, it's better to investigate and fix the root cause of the type mismatch instead of relying on a type assertion.
Please consider the following:
- Analyze why
tsconfigPath
is being treated asstring | nullish
instead ofstring | undefined
. - Ensure that the type definitions for
tsconfigPath
are consistent throughout the codebase. - Consider updating the type definitions or the logic that assigns the value to
tsconfigPath
to resolve the type mismatch.
Using a type assertion may hide potential issues and make the code less maintainable in the long run.
commit: |
…output when running in dev
126c4bd
to
fcf34c9
Compare
update
command, it now no longer shows "mismatch" errors when using in non-embedded mode.Summary by CodeRabbit
New Features
trigger.dev
CLI tool for better reliability and user experience, including a silent mode for commands.whoami
command and login process with optional silent operation.Bug Fixes